feat(security): add abuse controls and correlation IDs to mutation routes - #138
Open
woahwhattheheck wants to merge 17 commits into
Open
woahwhattheheck wants to merge 17 commits into
woahwhattheheck wants to merge 17 commits into
Conversation
…utes Bound transfer, user, quote, and admin mutation traffic with per-actor fixed-window limits, bounded limiter state, and safe 429 responses. Sanitize and propagate X-Request-Id / X-Correlation-Id without echoing secrets. Closes RemitFlow#131.
This was referenced Sep 24, 2026
Cache the earliest expiry and prune only when it becomes due. Keep the minimum exact across expired-key renewal, clock rollback and reset; preserve active budgets and 429 retry behavior. Regression: 128 rejected newcomers at 256 live keys previously traversed 65,536 entries; a deterministic one-table traversal bound now passes. Added expiry/retry, rollback and reset cases. Full suite: 271 passed, 0 failed, 0 skipped (original: 267 passed). Alternating real createApp HTTP runs (original/candidate/original/candidate), each with 10,000 admitted identities and three 2,000-request 429 batches at 32 connections. Across six batches per version: wall median 810.24 -> 336.59 ms; ranges 626.79-1050.47 -> 330.55-411.99 ms. Same-process client/server CPU median 849.97 -> 359.56 ms; ranges 653.17-1268.51 -> 351.43-578.41 ms. Local Node 24.19.0 / Express 4.22.2; explicit test-mode rate limiting, trusted proxy, error tracking disabled. Generated local load on a busy shared runtime; no absolute live throughput, deployed performance or hosted CI claim. Expiry-triggered cleanup remains O(maxKeys). Operation: REMIT138-SATURATED-LIMITER-20261003-8341
Assign correlation IDs and apply the global API limiter before JSON/form parsing. Malformed or oversized mutation bodies previously terminated before both middleware layers, allowing repeated parser work outside the documented request budget and returning uncorrelated errors. Preserve handler timeout, route-family limits and authentication behavior. An actual createApp HTTP regression sends malformed and oversized bodies with a two-request budget. The prior source returns 400,413,400,413; the repair returns 400,413,429,429, with request/correlation IDs on headers and errors plus global-policy and Retry-After metadata. Composed on e332e9b, retaining the saturated-limiter optimization and all four of its regressions. Exact composed source: npm test passes 272/272, 0 failed, 0 skipped on Node24.19.0. git diff --check passes. Operation: REMIT138-PARSER-BUDGET-20261003-443D
Add a runnable benchmark using only Node built-ins and the existing app dependencies. It starts the real createApp service on loopback, admits 10,000 identities, then records three 2,000-request rejection batches at 32 connections. Every rejection checks the global policy and correlation ID; the original exhausted budget must stay blocked. JSON retains raw samples, runtime versions and app/limiter source hashes. An optional checkout path supports alternating versions with the same script. Actual documented invocation completed on parent a41a01a, composing the existing saturated-table and parser-budget repairs: all 10,000 fills and 6,000 rejections matched, original budget remained blocked, exit 0. Client and server share one process; no live provider/deployed performance claim. Original before/after measurements remain scoped to e332e9b. Operation: REMIT138-BENCHMARK-REPRO-20261003-8341
Document trimming and first-valid header precedence, and distinguish the existing character/length check from secret or personal-data detection. Tell callers to use opaque IDs or accept a server-generated one because accepted values are echoed and logged. Only the correlation guide section changes. Existing limiter, parser, benchmark and source tests are retained. Source and full-text provider readback validate this continuation; no runtime, benchmark or maintained checks were replayed during the execution environment outage.
A printable token is not a safe request ID. Exclude inbound request/correlation IDs containing the Bearer or X-Admin-Token credential presented on the same request, before the value reaches response headers and request logging. Retain a separate safe fallback header or use the existing UUID generator. No authentication, actor-key, proxy-trust or rate-limit behavior changes; arbitrary unrelated PII is not claimed detectable. Focused native Node22.16.0 execution: test/requestIdCredentials.test.js before1pass/4fail, after5pass/0fail. Exercises both correlation headers, direct/embedded Bearer and admin tokens, independent fallback, ordinary precedence, syntax/length controls and fresh replacement IDs. Synthetic values only; no live credentials or account data. The complete source-pinned middleware and unchanged ids.js executed with request/response fixtures. The unavailable uuid package was supplied by an external preload mapping only uuid.v4 to node:crypto.randomUUID; that preload is not committed. No Express application, authentication flow, HTTP/provider, full-suite or deployed incident claim. With normal project dependencies: node --test test/requestIdCredentials.test.js. Composed on current dba2277, retaining the previous expiry implementation and its evidence. Existing PR138 and issue131 remain the submission path. No dependency, CI workflow or existing test removal.
Track the maximum fixed-window deadline and clear both bounded collections when no active budget remains. Preserve partial expiry, clock rollback, renewal and 429 behavior. The existing expiry/renewal selection passes four focused regressions, including one new whole-table boundary/reset case. Production-module comparisons retain identical response digests and eliminate 10,000 expired-entry Map deletions per full-table expiry. Exact paired timing data, admission-fill costs and replay script are retained in the abuse-controls guide; no deployed or total-throughput improvement is claimed. Preserves the preceding correlation guidance and all earlier source fixes.
Set X-RateLimit-Reset from the existing earliest-expiry index when the identity table is full. This prevents stacked global and mutation limiters from reporting different policies and deadlines. Three focused middleware-over-HTTP cases fail before the two-line repair and pass afterward, including standalone rejection and expiry reuse. Preserve all existing admission, expiry and retry behavior.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #131.
Adds production-shaped abuse controls on transfer, user, quote, and admin mutation routes, plus sanitized correlation-id propagation so accepted commands can be traced without leaking account secrets.
Note: #133 already has an open solid PR (#137), so this targets the unassigned fallback issue #131 instead of reminting lifecycle concurrency work.
What
maxKeyseviction) used for the global/apibudget and for route-family mutation budgets.GET /api/quote.TRUST_PROXYgates whetherX-Forwarded-Forcan influence client identity (off by default).X-Request-Id/X-Correlation-Id, reject unsafe values, and echo on both response headers and error envelopes.docs/ABUSE_CONTROLS.md.Why
High-volume retries and automated abuse can exhaust provider quotas and make incidents hard to correlate. Route-specific actor limits bound bursts; sanitized correlation ids keep every accepted command traceable without putting tokens or account data into logs or headers.
How tested
npm test— 267 passing (includes newtest/abuseControls.test.js).Retry-After, actor isolation, proxy-trust forgery rejection, distinct forwarded IPs when trusted,maxKeysbound under identity flood, correlation echo on success/429 without token leakage, unsafe inbound correlation replacement.Design tradeoffs
429, rate-limit headers, correlation headers) stays the same.NODE_ENV=testunlessforceInTest/ENABLE_RATE_LIMIT_IN_TEST=1, so the functional suite stays independent of the abuse budget while dedicated regressions still run.